Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement GetSConsVersion static method #4514

Merged
merged 2 commits into from
May 4, 2024

Conversation

Repiteo
Copy link
Contributor

@Repiteo Repiteo commented Apr 13, 2024

Solves a problem brought up in the SCons Discord about a lack of public api for the current version. Also adds some type hints, as a treat.

Contributor Checklist:

  • I have created a new test or updated the unit tests to cover the new/changed functionality.
  • I have updated CHANGES.txt (and read the README.rst)
  • I have updated the appropriate documentation

@Repiteo
Copy link
Contributor Author

Repiteo commented Apr 13, 2024

Not entirely convinced this implementation is the way to go; gonna look at some more examples before digging too deep into this. Marking as a draft for the time being.

@Repiteo Repiteo marked this pull request as draft April 13, 2024 21:54
@bdbaddog
Copy link
Contributor

Looks decent to me. Just needs a test. BTW version string can also have post1, and other version strings allowed by pypi..

@Repiteo
Copy link
Contributor Author

Repiteo commented Apr 13, 2024

That sort of inconsistency is part of why I think a more thorough implementation would be preferable. Something more along the lines of how python has the more robust sys.version_info. Being able to account for fringe situations would make this feel like a proper addition, instead of just making it less hacky to grab an existing piece of api.

@bdbaddog
Copy link
Contributor

I'd think you'd most likely be checking for major.minor.patch and the rest could be ignored?
At least for SCons releases, post releases are usually just packaging issues. I'd never release a post with functional differences.

@Repiteo
Copy link
Contributor Author

Repiteo commented Apr 13, 2024

I suppose that's fair, yeah. If I wanted to brainstorm something more, that'd probably go beyond the original scope of this PR. I'll get some tests up and running then undraft.

@Repiteo Repiteo marked this pull request as ready for review April 13, 2024 22:48
@bdbaddog
Copy link
Contributor

Can you add a blurb in the manpage and (maybe) users guide?
Here's where the manpage blurb would go: https://github.com/SCons/scons/blob/master/SCons/Script/SConscript.xml#L109

User's guide: https://github.com/SCons/scons/blob/master/doc/user/misc.xml#L112

@mwichmann
Copy link
Collaborator

The test for EnsureSConsVersion isn't happy now...

 TypeError: SConsEnvironment.EnsureSConsVersion() missing 1 required positional argument: 'minor':
  File "/tmp/testcmd.12132.jsexfe1p/SConstruct", line 2:
    env.EnsureSConsVersion(env.GetSConsVersion())


FAILED test of /home/runner/work/scons/scons/scripts/scons.py
	at line 673 of /home/runner/work/scons/scons/testing/framework/TestCommon.py (_complete)
	from line 775 of /home/runner/work/scons/scons/testing/framework/TestCommon.py (run)
	from line 458 of /home/runner/work/scons/scons/testing/framework/TestSCons.py (run)
	from line 72 of /home/runner/work/scons/scons/test/EnsureSConsVersion.py

@Repiteo
Copy link
Contributor Author

Repiteo commented Apr 14, 2024

Ahhh, silly me; I passed in the raw tuple instead of a broken-down version. Maybe I can add an overload to accept a tuple instead? Either way, it's an easy fix.

@Repiteo
Copy link
Contributor Author

Repiteo commented Apr 14, 2024

Haven't updated docs before so I'm unsure what I did wrong to make the test fail.

EDIT: Oh, duh. Needed to upload the generated files too. Probably.

EDIT2: Nevermind, I'm lost.

@bdbaddog bdbaddog merged commit 5f07b5d into SCons:master May 4, 2024
4 of 5 checks passed
@mwichmann mwichmann added this to the 4.8 milestone May 5, 2024
@Repiteo Repiteo deleted the GetSConsVersion branch July 5, 2024 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants